Skip to content

Add docs feature#21

Closed
ascjones wants to merge 13 commits intomainfrom
aj-docs-feature
Closed

Add docs feature#21
ascjones wants to merge 13 commits intomainfrom
aj-docs-feature

Conversation

@ascjones
Copy link
Copy Markdown
Contributor

Add a docs feature which when enabled will cause docs to be captured.

Adds a builder method to types with docs (StorageEntryMetadata and PalletConstantMetadata), which becomes an inline no-op when the feature is disabled. The compiler should then optimize away the static strngs.

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Jul 21, 2021

When would this feature be disabled?

@ascjones
Copy link
Copy Markdown
Contributor Author

Using default-features = false.

Since the main motivation for disabling docs is to reduce wasm size, it is assumed that for std builds it is okay to have it enabled by default.

That said maybe that assumption is not always true and it should be opt-in.

@ascjones
Copy link
Copy Markdown
Contributor Author

When would this feature be disabled?

Sorry I think I misunderstood what you meant here, you are asking why you would want to disable docs at all. It would be for wasm size because now all scale-info types come with docs attached with this feature enabled.

I am running benchmarks today to measure the difference, but as mentioned here #6 (comment), ideally we would have it enabled.

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Jul 21, 2021

Ahh all scale-info types would provide docs as well. Okay. However, I'm not sure we should use a feature flag for this. The problem I see is that feature flags are very hard to "control" in rust, like they easily leak when you enable them somewhere.

@ascjones
Copy link
Copy Markdown
Contributor Author

Sure but what alternative is there to avoid the doc strings being captured?

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Jul 21, 2021

Can we not disable the feature at runtime?

@ascjones
Copy link
Copy Markdown
Contributor Author

How do you mean?

We could indeed remove already captured docs at runtime. But the docs are captured statically by the macro so the strings will already be included in the binary.

Perhaps another option is to store the actual doc strings in a custom section, and have lookups into that. Then some wasm post-processing can remove the custom section if required e.g.

#[link_section = "docs"]
static MY_TYPE_DOCS: &[&'static str] = &["My type doc", "My field doc"];

Just thinking out loud, I am not sure whether that will work or not.

With all that said, if the wasm blob sizes are still manageable with docs included then we can remove this feature entirely.

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Jul 22, 2021

We could indeed remove already captured docs at runtime. But the docs are captured statically by the macro so the strings will already be included in the binary.

Ahh yeah, makes sense 👍

@ascjones
Copy link
Copy Markdown
Contributor Author

We might be able to close this if we go with my current solution, which is only to include the required docs in metadata: paritytech/substrate#8615 (comment), depends on paritytech/scale-info#118

@olanod
Copy link
Copy Markdown

olanod commented Jul 27, 2021

Even for few Ks I'd say is worth allowing to opt out :)

Copy link
Copy Markdown
Contributor

@gilescope gilescope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gilescope
Copy link
Copy Markdown
Contributor

I see paritytech/scale-info#118 is merged...

@ascjones
Copy link
Copy Markdown
Contributor Author

Closing as probably this isn't needed if it's been sitting here this long...

@ascjones ascjones closed this Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants